Bug fix for creating new racks on top of previous real racks - #1001
Conversation
aschmidt34
left a comment
aschmidt34
left a comment
There was a problem hiding this comment.
Made a few comments. Approved as none of these issues are breaking.
| const changeRack = async (newType: RackChangeOption): Promise<string | null> => { | ||
| let {value: rackChangeValue, label: rackLabel} = newType; | ||
| const { value: rackChangeValue } = newType; | ||
| const originalSelectedCage = selectedObj as Cage; |
There was a problem hiding this comment.
'selectedObj' is typed 'SelectedObj | null' on line 151
Might want to guard against a null cast here? I see this is how you cast though in other parts of the module so this might be okay, up to you. I doubt 'selectedObj' can ever truly be null for this.
| const newObjId = generateUUID(); | ||
| newCageData = { objectId: newObjId, svgId: `cageSVG_${newObjId}` as CageSvgId }; | ||
| } else { | ||
| const prevCage = prevCages.find(pc => pc.positionId === c.positionId); |
There was a problem hiding this comment.
'prevCages' is populated by 'cagesInRackConfig' query which is keyed on 'rackChangeValue.rackObjectId', and the map iterates over 'rack.cages' (the old rack's cages) to find matching 'positionId' (in new rack cages).
If old rack has more items than the new rack or position numbering isn't 1:1, prevCages.find() returns undefined and prevCage.objectId throws.
Only marking this because your original code had a guard: 'prevCages.length > 0 ? ... : r.cages'
| setReloadRoom(roomToUpdate); | ||
| return roomToUpdate; | ||
| const { rackGroup, rack } = findCageInGroup(originalSelectedCage.svgId, localRoom.rackGroups); | ||
| const newUnitLocs = { ...unitLocs }; |
There was a problem hiding this comment.
This is a shallow copy, so the inner array 'newUnitLocs[key]' is referencing the same 'unitLocs[key]'. If you mutate one, it will effect both.
On line 1061, this inner array is updated, meaning the OG value is also updated, which might mutate the currently-rendered unitLocs in-place before setUnitLocs (which references the OG value) commits a new state (since newUnitLocs[key] and the OG unitLocs[key] are the same array reference.)
Not sure how setUnitLocs works or when it runs, this might not be an issue, just wanted you to be aware.
| }; | ||
| setReloadRoom(roomToUpdate); | ||
| return roomToUpdate; | ||
| const { rackGroup, rack } = findCageInGroup(originalSelectedCage.svgId, localRoom.rackGroups); |
There was a problem hiding this comment.
You removed the 'prevRoom => ...' from the passed in args here. In your other function calls, 'prevRoom' is always the most current state when the function is called.
With this new version, 'localRoom' is just a variable captured when the render happens, not when the function is called.
SCENARIO:
- User triggers setLocalRoom(update A) with moveObjLocation.
- Before React re-renders and changeRack gets the new localRoom, the user triggers changeRack (which reads the stale localRoom without 'update A').
- changeRack computes roomToUpdate based on the old room and calls setLocalRoom(roomToUpdate).
- Because setLocalRoom(roomToUpdate) passes a plain object instead of an updater function, it overwrites state directly with a room that doesn't include 'update A' and 'update A' changes are silently lost.
This is a race condition, and I doubt users will ever make a change before this can take effect, but just wanted you to be aware. changeRack awaits a network call (labkeyActionSelectWithPromise) before reading localRoom so there's a small chance at a race condition error here.
2 participants
Rationale
Users are currently unable to create a new rack on a rack that is currently real as well due to conflicting object ids. This patch ensures that new object ids are generated if a rack is created on an existing rack.
Related Pull Requests
Changes